Issuance module #599
Issuance module #599
Conversation
karzak
added
R4R
karzak
requested review from
rhuairahrighairidh,
fedekunze,
denalimarsh and
nddeluca
There was a problem hiding this comment.
Nice, looks solid. I tried to break stuff but couldn't find anything
Only request is tidying up module.go - the tx and query functions are empty.
some nits:
- an asset owner could mint & send coins to module accounts, which might cause problems elsewhere. If the owner is minting coins automatically, they won't be checking to see if a user has malliciously submitted a mod account address as the destination address.
- When blocked addr coins are seized, it could confuse the accountant managing the owner address. Maybe emitt an event when coins are seized to help track where they came from?
- some msg ValidateBasic tests would be nice
I had a think about ways to pause asset transactions, but the best I came up with was to seize everyone's coins when pause is activated, then pay them back when unpaused. But that's a bit extreme and doesn't handle coins being in other places like cdps.
|
|
||
| // Params governance parameters for the issuance module | ||
| type Params struct { | ||
| Assets Assets `json:"assets" yaml:"assets"` |
There was a problem hiding this comment.
ok, so I think Assets should be governance params. They should be set to the issuance module store and defined on genesis. If a new asset needs to be added/removed you can create a custom gov proposal type for both operations and create a custom handler to add/delete assets from the store.
This way you have fine-grained control over the validation of the assets, eg validate that users cannot add and delete assets on the same param change proposal. Also, param change proposals don't allow you to validate the new value with the previously stored ones.
There was a problem hiding this comment.
Assets already are governance params because they are part of the Params for the module, so we can use param-change proposals to add/remove/edit assets. I think this is sufficient for governance for now, can expand if necessary as scope becomes better defined.
|
Created an issue on cosmos-sdk for enabling/disabling coin transfers of individual coins. |
x/issuance/keeper/issuance.go
Outdated
| @@ -98,6 +100,33 @@ func (k Keeper) BlockAddress(ctx sdk.Context, denom string, owner, blockedAddres | |||
| return nil | |||
| } | |||
|
|
|||
| // UnblockAddress adds an address to the blocked list | |||
There was a problem hiding this comment.
| // UnblockAddress adds an address to the blocked list | |
| // UnblockAddress removes an address from the blocked list |
x/issuance/types/msg.go
Outdated
| Address sdk.AccAddress `json:"address" yaml:"address"` | ||
| } | ||
|
|
||
| // NewMsgUnblockAddress returns a new MsgIssueTokens |
There was a problem hiding this comment.
| // NewMsgUnblockAddress returns a new MsgIssueTokens | |
| // NewMsgUnblockAddress returns a new MsgUnblockAddress |
|
|
||
| // String implements fmt.Stringer | ||
| func (msg MsgUnblockAddress) String() string { | ||
| return fmt.Sprintf(`Unblock Address: |
Note: The v0.3 -> v0.8 migration tests will need to be updated with a custom app type for
v0.8, then that type should be used in the migration tests. Out of scope for this PR.